Spec insert >http to pro lang start #10187
Conversation
…of API calls into Language SDK. Started with python first - Added "Example_code in utils.rb" - Added Switch statement for example_code - implemented example_code.rb to hande logic for each language - example_code.mustache for templating - cat-allocation.md will be the starting test file
|
Thank you for submitting your PR. The PR states are In progress (or Draft) -> Tech review -> Doc review -> Editorial review -> Merged. Before you submit your PR for doc review, make sure the content is technically accurate. If you need help finding a tech reviewer, tag a maintainer. When you're ready for doc review, tag the assignee of this PR. The doc reviewer may push edits to the PR directly or leave comments and editorial suggestions for you to address (let us know in a comment if you have a preference). The doc reviewer will arrange for an editorial review. |
|
|
@nhtruong Would be fantastic if you could review this PR at your convenience. Thanks! |
| @@ -27,6 +27,40 @@ GET /_cat/allocation/{node_id} | |||
| ``` | |||
| <!-- spec_insert_end --> | |||
|
|
|||
| <!-- spec_insert_start | |||
| api: snapshot.restore | |||
There was a problem hiding this comment.
did you mean to use cat.allocation ?
There was a problem hiding this comment.
yea. To run test to make sure this works, i used the cat.allocation file to run multiple examples and forget to change it back when pushing. will fix
|
|
||
| require 'json' | ||
|
|
||
| class ExampleCode < BaseMustacheRenderer |
There was a problem hiding this comment.
- This class will become unmaintainable once we add more languages for this component. Each language's render logic should have its own renderer class. The ExampleCode class should only be responsible for collecting the rendered code examples from different languages and put them in
{% catpure %}and{% include %}blocks. - The rendering of each language example code is complex enough to warrant a dedicated mustache template instead of 20 string-manipulation methods that call one another like we see here (I have no idea where to start, nor a vague idea of what the output will look like after taking a squick scan of this file). It's worth spending some time reading the Mustache manual first (it's not long at all). So, for Python, create
example_code.python.mustachethat will be consumed byExampleCodePython < BaseMustacheRenderer - Don't forget to add tests ;-)
| end | ||
|
|
||
| def rest_lines | ||
| @args.raw['rest']&.split("\n")&.map(&:strip) || [] |
There was a problem hiding this comment.
Avoid accessing the raw attr of InsertArguments class like this. Instead, create InsertArguments#rest method. The purpose of this class is parsing the component args to be consumed by the render classes. So, this logic you have here is the responsibility of the InsertArguments class, not this Renderer class. This isolation of concerns principle makes code bases (esp large ones) more maintainable (from better testability and readability). It will also increase reusability (You don't have to keep parsing this arg at different places when they are used more than once)
| end | ||
|
|
||
| # Parses the body from the REST example (only for preserving raw formatting) | ||
| def body |
There was a problem hiding this comment.
Looks like this is a parsing logic for the rest arg as well. Once you have a proper InsetArguments#rest. This should be accessed through @args.rest[:body] or @args.rest.body depending on your implementation of #rest.
| <!-- spec_insert_start | ||
| api: snapshot.restore | ||
| component: example_code | ||
| rest: GET _nodes/stats/indices |
There was a problem hiding this comment.
(The comment below is not about this PR specifically but about the direction of this project)
This rest arg implies that the person editing this file must know how to use this API as they are also responsible for filling in this arg. This will be a tall order especially for more complex APIs with large request bodies (e.g. the search API). This will also cause other issues that this very project is meant to solve:
- Outdated docs: When an API is updated, say renaming a query string param that appears in the
restarg, the change in the spec will not be propagated to the doc due to the hardcoding of therestarg. - Human errors: There's no mechanism to make sure that human-input
restarg is without error. For example,GET _nodes/stats/indicesis used in thiscat-allocation.md:)) - Automation: In the final form of this project, when a new API is added, it will automatically generate an
mdfile for that API through the spec, including allspec-insertcomponents and their args. That means theexample_codecomponent should depend on args likerest
The OpenSearch API spec repo has very comprehensive tests for most APIs. These tests are run against actual OS clusters and reviewed by experts. They are also updated along with the spec. This project should construct code examples from those tests to resolve the problems mentioned above. This approach shifts most of the burdens onto the engineer(s) maintaining spec-insert, but I'd argue that it's a much more realistic option in the long run. I doubt the docs team will have the time to construct the rest arg for every API in OpenSearch, and even then they will still have to make sure that updates on the spec will be reflected in the APIs with code_example components.
There was a problem hiding this comment.
@dblock what do you think?
There was a problem hiding this comment.
Thank you for the feedback, i will take these into consideration moving forward and work towards making the improvements.
| call_code | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Some advice regarding good coding practice (not specific to Ruby):
- Keep each method short and simple: You can't see the beginning and the end of a method on the same screen (i.e. you have to scroll), the method is too long!
- Limit the number of methods in a class/file: The human memory's buffer is very limited, if you have 20 methods that call one another, we start losing track of what your code is doing (And your AI agents will start to hallucinate)
These seems to contradict each other since breaking a big method into smaller ones will increase the number of methods. There's an art in finding a balance for this that you will mater overtime. These go hand in hand with the isolation of concerns and do not repeat yourself (DRY) principles: You have less methods and shorter methods when you apply these principles properly :)
|
@ReveristRealm Please consider matching the example requests to their corresponding API specs using regular expressions. This will greatly simplify this task: there would be no human intervention required to put in tags manually. Thanks! |
kolchfa-aws
added
Tech review
Description
Converting the Opensearch APIS into the respective language SDK( Starting with Python first)
Issues Resolved
Code example only showing raw API or curl
Version
List the OpenSearch version to which this PR applies, e.g. 2.14, 2.12--2.14, or all.
Frontend features
If you're submitting documentation for an OpenSearch Dashboards feature, add a video that shows how a user will interact with the UI step by step. A voiceover is optional.
Checklist
For more information on following Developer Certificate of Origin and signing off your commits, please check here.